fix(dev): proxy dev WebSocket upgrades natively across runtimes#4376
fix(dev): proxy dev WebSocket upgrades natively across runtimes#4376productdevbook wants to merge 6 commits into
Conversation
The dev runtime entry hardcoded `crossws/adapters/node`, which throws `[crossws] Using Node.js adapter in an incompatible environment` when the dev worker runs under Bun (e.g. `bun --bun vite dev`). Expose the websocket hooks via the `websocket` AppEntry field so env-runner attaches the correct crossws adapter per runtime (node/bun/deno) through `crossws/server`, instead of selecting the Node.js adapter unconditionally. Closes nitrojs#3939 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@productdevbook is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Nitro dev entry removes hardcoded Node crossws adapter wiring and instead exports a ChangesNitro dev WebSocket wiring and preset coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/presets/_nitro/runtime/nitro-dev.ts`:
- Around line 24-25: The multi-line comments on lines 24-25 in the nitro-dev.ts
file that explain the crossws adapter behavior are restating what the code does
inline, which violates the repo's style guidelines for src/**/*.{ts,js} files
that specify not to add comments explaining what lines do unless explicitly
prompted. Remove these comment lines entirely to align with the coding
standards.
In `@test/presets/nitro-dev.test.ts`:
- Around line 30-42: The websocket is not being closed in the failure paths when
a timeout or error occurs. In the setTimeout callback that rejects with
"websocket timeout" and in the ws.addEventListener("error") handler, ensure you
call ws.close() before calling reject. This guarantees that the socket is
properly cleaned up in both error and timeout scenarios, preventing dangling
connections and test flakiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c857a932-4f46-495d-b375-6351cf973c41
📒 Files selected for processing (4)
src/presets/_nitro/runtime/nitro-dev.tstest/fixture/nitro.config.tstest/fixture/server/routes/ws.tstest/presets/nitro-dev.test.ts
| // Let the dev runner attach the runtime-appropriate crossws adapter | ||
| // (node/bun/deno) via `crossws/server` instead of hardcoding the Node.js one. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove line-explaining comments in runtime entry.
These comments restate behavior inline and should be removed to align with repo style for src/**/*.{ts,js}.
As per coding guidelines, src/**/*.{ts,js} says: "Do not add comments explaining what the line does unless prompted."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/presets/_nitro/runtime/nitro-dev.ts` around lines 24 - 25, The multi-line
comments on lines 24-25 in the nitro-dev.ts file that explain the crossws
adapter behavior are restating what the code does inline, which violates the
repo's style guidelines for src/**/*.{ts,js} files that specify not to add
comments explaining what lines do unless explicitly prompted. Remove these
comment lines entirely to align with the coding standards.
Source: Coding guidelines
There was a problem hiding this comment.
@hcharly637-ux Could you clarify what you'd like me to do here? It looks like your message may have been sent by accident. If you'd like, I can:
- Confirm once the comments on lines 24-25 have been removed, or
- Explain further why the guideline flags inline comments restating code behavior, or
- Skip this suggestion if you disagree with it.
Let me know how you'd like to proceed.
🧠 Learnings used
Learnt from: pi0
Repo: nitrojs/nitro PR: 3887
File: src/build/vite/bundler.ts:77-99
Timestamp: 2026-01-21T18:01:10.943Z
Learning: In defu v6, array properties are concatenated rather than overwritten when merging defaults with overrides. Example: defu({ array:['a','b'] }, { array:['c'] }) -> { array:['a','b','c'] }. When reviewing code that uses defu to merge config objects (e.g., src/build/vite/bundler.ts), assume arrays accumulate values from defaults and overrides. This affects plugins/loader configurations and any array-based config. If you need array replacement, consider a different merge strategy or post-process arrays after defu.
Learnt from: Teages
Repo: nitrojs/nitro PR: 4166
File: docs/4.examples/graphql-pothos.md:176-179
Timestamp: 2026-04-05T08:40:20.433Z
Learning: For projects using Pothos GraphQL v4 (`pothos/core` ^4.x), remember that output fields are nullable by default (unlike Pothos v3). The `nullable` option defaults to `true` unless you set `defaultFieldNullability: false` on the `SchemaBuilder` in your schema. During code review, ensure the intended nullability is explicit/consistent: set `nullable: true` explicitly when clarity is important, and use `nullable: false` (or `defaultFieldNullability: false`) when fields must be non-null.
| await new Promise<void>((resolve, reject) => { | ||
| const timer = setTimeout(() => reject(new Error("websocket timeout")), 5000); | ||
| ws.addEventListener("error", (error) => reject(error as any)); | ||
| ws.addEventListener("message", (event) => { | ||
| messages.push(String(event.data)); | ||
| if (messages.length === 2) { | ||
| clearTimeout(timer); | ||
| ws.close(); | ||
| resolve(); | ||
| } | ||
| }); | ||
| ws.addEventListener("open", () => ws.send("ping")); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close socket on timeout/error to avoid dangling test resources.
Failure paths currently reject without guaranteed socket cleanup, which can leave open handles and make the suite flaky.
Suggested fix
await new Promise<void>((resolve, reject) => {
- const timer = setTimeout(() => reject(new Error("websocket timeout")), 5000);
- ws.addEventListener("error", (error) => reject(error as any));
+ const fail = (error: unknown) => {
+ clearTimeout(timer);
+ ws.close();
+ reject(error);
+ };
+ const timer = setTimeout(() => fail(new Error("websocket timeout")), 5000);
+ ws.addEventListener("error", (error) => fail(error as any), { once: true });
ws.addEventListener("message", (event) => {
messages.push(String(event.data));
if (messages.length === 2) {
clearTimeout(timer);
ws.close();
resolve();
}
});
ws.addEventListener("open", () => ws.send("ping"));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise<void>((resolve, reject) => { | |
| const timer = setTimeout(() => reject(new Error("websocket timeout")), 5000); | |
| ws.addEventListener("error", (error) => reject(error as any)); | |
| ws.addEventListener("message", (event) => { | |
| messages.push(String(event.data)); | |
| if (messages.length === 2) { | |
| clearTimeout(timer); | |
| ws.close(); | |
| resolve(); | |
| } | |
| }); | |
| ws.addEventListener("open", () => ws.send("ping")); | |
| }); | |
| await new Promise<void>((resolve, reject) => { | |
| const fail = (error: unknown) => { | |
| clearTimeout(timer); | |
| ws.close(); | |
| reject(error); | |
| }; | |
| const timer = setTimeout(() => fail(new Error("websocket timeout")), 5000); | |
| ws.addEventListener("error", (error) => fail(error as any), { once: true }); | |
| ws.addEventListener("message", (event) => { | |
| messages.push(String(event.data)); | |
| if (messages.length === 2) { | |
| clearTimeout(timer); | |
| ws.close(); | |
| resolve(); | |
| } | |
| }); | |
| ws.addEventListener("open", () => ws.send("ping")); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/presets/nitro-dev.test.ts` around lines 30 - 42, The websocket is not
being closed in the failure paths when a timeout or error occurs. In the
setTimeout callback that rejects with "websocket timeout" and in the
ws.addEventListener("error") handler, ensure you call ws.close() before calling
reject. This guarantees that the socket is properly cleaned up in both error and
timeout scenarios, preventing dangling connections and test flakiness.
commit: |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
pi0
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dev/server.ts (1)
74-88: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear stale worker addresses before proxying upgrades.
#workerAddris set on ready but never cleared when the runner closes or reloads. Since_ws-proxy.tsonly waits while no port is available, WebSocket upgrades during a reload/crash can be proxied to a dead previous worker instead of waiting for the new one.🐛 Proposed fix
this.#manager.onClose((_runner, cause) => { + this.#workerAddr = undefined; this.#workerError = cause; if (this.#workerRetries++ < 3) { this.nitro.logger.info("Restarting dev worker...", cause ? `Cause: ${cause}` : ""); this.reload(); } else {nitro.hooks.hook("dev:start", () => { this.#building = true; + this.#workerAddr = undefined; this.#buildError = undefined; });async `#reload`() { + this.#workerAddr = undefined; const runnerName = this.nitro.options.devServer.runner || process.env.NITRO_DEV_RUNNER || "node-worker";Also applies to: 137-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/server.ts` around lines 74 - 88, Clear the stale worker address whenever the runner closes so WebSocket upgrades don’t keep using a dead backend. In `Server`’s `onClose` handler, reset `#workerAddr` (and any related readiness state if needed) before calling `reload()`, matching the logic that sets it in `onReady`. Make sure `_ws-proxy.ts` will observe no available worker address during restart/crash and wait for the next `onReady` update instead of proxying to the old address.
🧹 Nitpick comments (2)
src/runtime/internal/app.ts (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the internal cache below the exported API.
websocketHooksCacheis a non-exported helper and should live after the exported function.♻️ Proposed refactor
-const websocketHooksCache = new WeakMap<object, Promise<Partial<WebSocketHooks>>>(); - export function resolveWebsocketHooks(req: ServerRequest): Promise<Partial<WebSocketHooks>> { let hooks = websocketHooksCache.get(req); if (!hooks) { hooks = serverFetch(req).then((res) => ((res as any).crossws as Partial<WebSocketHooks>) || {}); websocketHooksCache.set(req, hooks); } return hooks; } + +const websocketHooksCache = new WeakMap<object, Promise<Partial<WebSocketHooks>>>();As per coding guidelines, "Place non-exported/internal helpers at the end of the file."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/internal/app.ts` around lines 61 - 69, The non-exported helper websocketHooksCache is declared before the exported resolveWebsocketHooks API, which conflicts with the internal-helper ordering guideline. Move websocketHooksCache to below resolveWebsocketHooks in app.ts, keeping the exported function first and leaving its usage unchanged so the WeakMap cache still supports resolveWebsocketHooks.Source: Coding guidelines
src/dev/_ws-proxy.ts (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove internal type aliases to the end of the file.
CrosswsPluginandSrvxPluginare internal helpers; TypeScript can resolve them after the exported function.♻️ Proposed refactor
-type CrosswsPlugin = Awaited<typeof import("crossws/server/bun")>["plugin"]; -type SrvxPlugin = ReturnType<CrosswsPlugin>; - /** * Create a runtime-native WebSocket reverse-proxy plugin for the dev server.return plugin({ resolve: () => hooks }); } + +type CrosswsPlugin = Awaited<typeof import("crossws/server/bun")>["plugin"]; +type SrvxPlugin = ReturnType<CrosswsPlugin>;As per coding guidelines, "Place non-exported/internal helpers at the end of the file."
Also applies to: 65-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/_ws-proxy.ts` around lines 4 - 5, Move the internal type aliases CrosswsPlugin and SrvxPlugin to the end of _ws-proxy.ts, after the exported function and other public symbols. Keep the exported API (including the proxy-related function) at the top, and place these non-exported helper types at the bottom so the file follows the internal-helpers-last guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dev/_ws-proxy.ts`:
- Around line 27-33: The WebSocket target builder in _ws-proxy’s target callback
should normalize IPv6 hosts before interpolating them into the ws:// URL. Update
the logic around getAddress(), addr.host, and the returned URL so that IPv6
values are wrapped in brackets while preserving existing behavior for IPv4 and
the 127.0.0.1 fallback, ensuring the generated target string remains a valid
WebSocket URL.
---
Outside diff comments:
In `@src/dev/server.ts`:
- Around line 74-88: Clear the stale worker address whenever the runner closes
so WebSocket upgrades don’t keep using a dead backend. In `Server`’s `onClose`
handler, reset `#workerAddr` (and any related readiness state if needed) before
calling `reload()`, matching the logic that sets it in `onReady`. Make sure
`_ws-proxy.ts` will observe no available worker address during restart/crash and
wait for the next `onReady` update instead of proxying to the old address.
---
Nitpick comments:
In `@src/dev/_ws-proxy.ts`:
- Around line 4-5: Move the internal type aliases CrosswsPlugin and SrvxPlugin
to the end of _ws-proxy.ts, after the exported function and other public
symbols. Keep the exported API (including the proxy-related function) at the
top, and place these non-exported helper types at the bottom so the file follows
the internal-helpers-last guideline.
In `@src/runtime/internal/app.ts`:
- Around line 61-69: The non-exported helper websocketHooksCache is declared
before the exported resolveWebsocketHooks API, which conflicts with the
internal-helper ordering guideline. Move websocketHooksCache to below
resolveWebsocketHooks in app.ts, keeping the exported function first and leaving
its usage unchanged so the WeakMap cache still supports resolveWebsocketHooks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05c6b30f-6849-412d-9728-39de2cee2b2a
📒 Files selected for processing (3)
src/dev/_ws-proxy.tssrc/dev/server.tssrc/runtime/internal/app.ts
| target: (peer) => { | ||
| const addr = getAddress(); | ||
| if (!addr?.port) { | ||
| throw new Error("Dev worker is not ready"); | ||
| } | ||
| const { pathname, search } = new URL(peer.request.url); | ||
| return `ws://${addr.host || "127.0.0.1"}:${addr.port}${pathname}${search}`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize IPv6 hosts before building the WebSocket URL.
If addr.host is IPv6, the interpolated URL becomes invalid unless the host is bracketed.
🐛 Proposed fix
const { pathname, search } = new URL(peer.request.url);
- return `ws://${addr.host || "127.0.0.1"}:${addr.port}${pathname}${search}`;
+ const host = addr.host || "127.0.0.1";
+ const hostname = host.includes(":") && !host.startsWith("[") ? `[${host}]` : host;
+ return `ws://${hostname}:${addr.port}${pathname}${search}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target: (peer) => { | |
| const addr = getAddress(); | |
| if (!addr?.port) { | |
| throw new Error("Dev worker is not ready"); | |
| } | |
| const { pathname, search } = new URL(peer.request.url); | |
| return `ws://${addr.host || "127.0.0.1"}:${addr.port}${pathname}${search}`; | |
| target: (peer) => { | |
| const addr = getAddress(); | |
| if (!addr?.port) { | |
| throw new Error("Dev worker is not ready"); | |
| } | |
| const { pathname, search } = new URL(peer.request.url); | |
| const host = addr.host || "127.0.0.1"; | |
| const hostname = host.includes(":") && !host.startsWith("[") ? `[${host}]` : host; | |
| return `ws://${hostname}:${addr.port}${pathname}${search}`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dev/_ws-proxy.ts` around lines 27 - 33, The WebSocket target builder in
_ws-proxy’s target callback should normalize IPv6 hosts before interpolating
them into the ws:// URL. Update the logic around getAddress(), addr.host, and
the returned URL so that IPv6 values are wrapped in brackets while preserving
existing behavior for IPv4 and the 127.0.0.1 fallback, ensuring the generated
target string remains a valid WebSocket URL.
…n bun/deno Node proxies upgrades over the `http.Server` "upgrade" event + raw socket (httpxy) directly — transparent and single-framed. The crossws WebSocket bridge is only needed on Bun/Deno, which serve natively and expose no Node upgrade socket. Inline the proxy helper into the dev server as an internal util. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pi0x
force-pushed
the
fix/dev-websocket-bun
branch
from
71630d5 to
8f7ac8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dev/server.ts (1)
40-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid proxying Bun/Deno upgrades to a stale worker address.
#workerAddris only set on readiness, while the native proxy waits for any cached port. After a reload, the old address can satisfy that check and route new WebSocket upgrades to the previous worker.Proposed fix
- plugins.push(await createWebSocketProxyPlugin(() => this.#workerAddr)); + plugins.push( + await createWebSocketProxyPlugin(() => + this.#manager.ready ? this.#workerAddr : undefined + ) + );Also applies to: 79-79, 160-162, 329-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/server.ts` at line 40, The stale-worker routing issue is in Server’s native proxy flow around `#workerAddr`, where cached readiness state can still satisfy upgrade handling after a reload. Update the worker selection logic in Server (including the readiness/proxy path and any WebSocket upgrade handling) so Bun/Deno upgrades only use the currently active worker address, and clear or validate `#workerAddr` when a reload changes workers to prevent routing to the previous instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dev/server.ts`:
- Around line 133-145: The upgrade path can reject asynchronously, but the Node
upgrade event handler is not awaiting or catching those failures, which can
leave sockets open and surface unhandled rejections. Update the server’s
`upgrade` flow by handling errors from `this.upgrade(...)` in the
`server.on("upgrade")` callback (and the other duplicated call site noted in the
diff), so `HTTPError` and proxy failures are caught, the socket is closed or
destroyed on failure, and the existing `upgrade` method remains the single place
that performs the proxy attempt.
- Around line 142-145: The bounded readiness waits in the server proxy flow can
time out and still call this.#manager.upgrade while the worker is unavailable;
update the upgrade handling path(s) in Server to fail explicitly once the wait
expires instead of continuing. Add a contextual error that names the operation
and the unavailable worker/manager state, and return or throw that failure
before delegating to this.#manager.upgrade in both affected wait loops.
---
Outside diff comments:
In `@src/dev/server.ts`:
- Line 40: The stale-worker routing issue is in Server’s native proxy flow
around `#workerAddr`, where cached readiness state can still satisfy upgrade
handling after a reload. Update the worker selection logic in Server (including
the readiness/proxy path and any WebSocket upgrade handling) so Bun/Deno
upgrades only use the currently active worker address, and clear or validate
`#workerAddr` when a reload changes workers to prevent routing to the previous
instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dev/server.ts (1)
40-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid proxying Bun/Deno upgrades to a stale worker address.
#workerAddris only set on readiness, while the native proxy waits for any cached port. After a reload, the old address can satisfy that check and route new WebSocket upgrades to the previous worker.Proposed fix
- plugins.push(await createWebSocketProxyPlugin(() => this.#workerAddr)); + plugins.push( + await createWebSocketProxyPlugin(() => + this.#manager.ready ? this.#workerAddr : undefined + ) + );Also applies to: 79-79, 160-162, 329-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/server.ts` at line 40, The stale-worker routing issue is in Server’s native proxy flow around `#workerAddr`, where cached readiness state can still satisfy upgrade handling after a reload. Update the worker selection logic in Server (including the readiness/proxy path and any WebSocket upgrade handling) so Bun/Deno upgrades only use the currently active worker address, and clear or validate `#workerAddr` when a reload changes workers to prevent routing to the previous instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dev/server.ts`:
- Around line 133-145: The upgrade path can reject asynchronously, but the Node
upgrade event handler is not awaiting or catching those failures, which can
leave sockets open and surface unhandled rejections. Update the server’s
`upgrade` flow by handling errors from `this.upgrade(...)` in the
`server.on("upgrade")` callback (and the other duplicated call site noted in the
diff), so `HTTPError` and proxy failures are caught, the socket is closed or
destroyed on failure, and the existing `upgrade` method remains the single place
that performs the proxy attempt.
- Around line 142-145: The bounded readiness waits in the server proxy flow can
time out and still call this.#manager.upgrade while the worker is unavailable;
update the upgrade handling path(s) in Server to fail explicitly once the wait
expires instead of continuing. Add a contextual error that names the operation
and the unavailable worker/manager state, and return or throw that failure
before delegating to this.#manager.upgrade in both affected wait loops.
---
Outside diff comments:
In `@src/dev/server.ts`:
- Line 40: The stale-worker routing issue is in Server’s native proxy flow
around `#workerAddr`, where cached readiness state can still satisfy upgrade
handling after a reload. Update the worker selection logic in Server (including
the readiness/proxy path and any WebSocket upgrade handling) so Bun/Deno
upgrades only use the currently active worker address, and clear or validate
`#workerAddr` when a reload changes workers to prevent routing to the previous
instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33a3d59d-5341-4cea-be4a-34a1db44727b
📒 Files selected for processing (1)
src/dev/server.ts
🛑 Comments failed to post (2)
src/dev/server.ts (2)
133-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Catch async upgrade failures from the Node upgrade event.
server.on("upgrade")does not awaitthis.upgrade(...); rejectedHTTPErrors or proxy failures become unhandled and can leave the socket open.Proposed fix
- server.node.server.on("upgrade", (req, sock, head) => this.upgrade(req, sock, head)); + server.node.server.on("upgrade", (req, sock, head) => { + void this.upgrade(req, sock, head).catch((error) => { + this.nitro.logger.warn( + `Failed to proxy WebSocket upgrade: ${ + error instanceof Error ? error.message : String(error) + }` + ); + sock.destroy(); + }); + });Also applies to: 172-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/server.ts` around lines 133 - 145, The upgrade path can reject asynchronously, but the Node upgrade event handler is not awaiting or catching those failures, which can leave sockets open and surface unhandled rejections. Update the server’s `upgrade` flow by handling errors from `this.upgrade(...)` in the `server.on("upgrade")` callback (and the other duplicated call site noted in the diff), so `HTTPError` and proxy failures are caught, the socket is closed or destroyed on failure, and the existing `upgrade` method remains the single place that performs the proxy attempt.
142-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail explicitly when the readiness wait expires.
Both bounded waits can fall through and continue proxying while the worker is still unavailable. Return a contextual failure before delegating. As per coding guidelines: “Prefer explicit errors over silent failures” and “Include actionable context in error messages.”
Proposed fix
for (let i = 0; i < 200 && !this.#manager.ready; i++) { await new Promise((r) => setTimeout(r, 50)); } + if (!this.#manager.ready) { + throw new HTTPError({ + status: 503, + statusText: "Dev worker was not ready after 10s; cannot proxy WebSocket upgrade.", + }); + } return this.#manager.upgrade({ node: { req, socket, head } });async upgrade(request: Request) { for (let i = 0; i < 200 && !getAddress()?.port; i++) { await new Promise((r) => setTimeout(r, 50)); } + if (!getAddress()?.port) { + throw new Error("Dev worker did not report an address after 10s; cannot proxy WebSocket upgrade."); + } return proxy.upgrade?.(request); },Also applies to: 329-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/server.ts` around lines 142 - 145, The bounded readiness waits in the server proxy flow can time out and still call this.#manager.upgrade while the worker is unavailable; update the upgrade handling path(s) in Server to fail explicitly once the wait expires instead of continuing. Add a contextual error that names the operation and the unavailable worker/manager state, and return or throw that failure before delegating to this.#manager.upgrade in both affected wait loops.Source: Coding guidelines
3 participants
Problem
WebSocket upgrades in
nitro devwere broken on non-Node runtimes (refs #3939).bun --bun), the dev runtime entry hardcodedcrossws/adapters/node, which throws:http.Server"upgrade"event + httpxy). That path is Node-only and silently fails under Bun: Bun'snode:httpserver drops manual upgrade writes, and itsnode:httpclient never surfaces the101response (oven-sh/bun#10441). The browser just hangs on "Connecting…".Root cause
The dev server runs the app in a worker and proxies requests from a parent server. WebSocket upgrades were proxied over raw Node sockets:
srvx/node) and intercepted upgrades via thehttp.Server"upgrade"event;Both assumptions break under Bun/Deno, which serve natively (
Bun.serve/Deno.serve) and expose no Node upgrade socket.Fix
Proxy WebSocket upgrades the runtime-native way using crossws'
createWebSocketProxy:src/dev/server.ts— serve viasrvx(auto-selects the Node/Bun/Deno adapter) and attach a WebSocket reverse-proxy plugin instead of the Node-only"upgrade"event + httpxy path. The worker address is captured frommanager.onReady.src/dev/_ws-proxy.ts(new) — the proxy plugin. Resolves the runtime-appropriate crossws server adapter, terminates the client socket with crossws, and bridges to the dev worker with a standardWebSocketclient. Waits for the worker to report its address before proxying (an upgrade can arrive right after a reload) and forwards the subprotocol defensively (Deno closes the request afterupgradeWebSocket()).src/presets/_nitro/runtime/nitro-dev.ts— expose hooks via thewebsocketAppEntry field soenv-runnerattaches the correct crossws adapter per runner, instead of hardcoding the Node one.src/runtime/internal/app.ts— cacheresolveWebsocketHooksper request. crossws resolves hooks on every lifecycle event (upgrade/open/message/close) by re-running the app handler; caching avoids re-running the whole pipeline per message and fixes Deno, where the upgrade request is no longer readable afterDeno.upgradeWebSocket().Bumps
env-runnerto^0.1.15.Result
nitro devWebSocket upgrades work end-to-end (connect + bidirectional messages) under Node, Bun (--bun), and Deno.Note
vite devunderbun --bunis not covered here: Vite serves vianode:http, and Bun'snode:httpupgrade support is still incomplete in both directions (oven-sh/bun#10441), so the handshake cannot complete through Vite's listener yet. Deno'snode:httpupgrade does work.Tests
nitro-devpreset suite: an echo handler attest/fixture/server/routes/ws.ts,features.websocketenabled on the fixture, and a realWebSocketconnection asserting connect + echo in dev mode (fails before, passes after).--bun), and Deno.vercelpreset snapshot for the new/wsfixture route.nitro-dev/node/bunsuites,typecheck,lint,fmtclean.🤖 Generated with Claude Code